-
Notifications
You must be signed in to change notification settings - Fork 140
Add support for Ephemeral TaskLists #1449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (83.11%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
4ef39f0
to
79c12c6
Compare
if options != nil { | ||
return FeatureFlags{ | ||
WorkflowExecutionAlreadyCompletedErrorEnabled: options.FeatureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled, | ||
PollerAutoScalerEnabled: options.FeatureFlags.PollerAutoScalerEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a comment: this flag is no longer used and I'll delete it in the next PR
test/integration_test.go
Outdated
|
||
func (ts *IntegrationTestSuite) TestSession_Ephemeral() { | ||
// Ephemeral TaskList is enabled by the test name | ||
ts.TestSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can describe tasklist to ensure the tasklist is gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TTL is rather high unfortunately. We can't really validate that behavior without inspecting the database directly.
test/workflow_test.go
Outdated
} | ||
defer workflow.CompleteSession(sessionCtx) | ||
var expected string | ||
if err = workflow.ExecuteActivity(ctx, "Prefix_ToUpper", "hello").Get(ctx, &expected); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be sessionCtx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks
func (ts *IntegrationTestSuite) TestSession_Ephemeral() { | ||
// Ephemeral TaskList is enabled by the test name | ||
ts.TestSession() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could do verification
func (ts *IntegrationTestSuite) TestSession_Ephemeral() { | |
// Ephemeral TaskList is enabled by the test name | |
ts.TestSession() | |
} | |
func (ts *IntegrationTestSuite) TestSession_Ephemeral() { | |
// Ephemeral TaskList is enabled by the test name | |
execution, err := ts.executeWorkflow("test-session", ts.workflows.Session, nil) | |
ts.NoError(err) | |
taskList := ts.getActivityTaskList(execution) | |
ts.Equal(shared.TaskListKindEphemeral, taskList.GetKind()) | |
taskList = ts.describeTaskList(taskList.GetName()) | |
ts.Equal(shared.TaskListKindEphemeral, taskList.GetKind()) | |
} | |
func (ts *IntegrationTestSuite) getActivityTaskList(execution *workflow.Execution) *shared.TaskList { | |
iter := ts.libClient.GetWorkflowHistory(context.Background(), execution.ID, execution.RunID, false, shared.HistoryEventFilterTypeAllEvent) | |
for iter.HasNext() { | |
event, err := iter.Next() | |
ts.Require().NoError(err) | |
if event.GetActivityTaskScheduledEventAttributes() != nil { | |
return event.GetActivityTaskScheduledEventAttributes().GetTaskList() | |
} | |
} | |
return nil | |
} | |
func (ts *IntegrationTestSuite) describeTaskList(taskListName string) *shared.TaskList { | |
descResp, err := ts.libClient.DescribeTaskList(context.Background(), taskListName, shared.TaskListTypeActivity) | |
ts.Require().NoError(err) | |
return descResp.GetTaskList() | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and added. The workflow now returns the TaskList the activity ran on, which we can check.
c4aa7f0
to
cd46896
Compare
Update Sessions to use Ephemeral TaskLists behind a feature flag. This ensures that the per-host TaskList is automatically removed once it is no longer used. This should only be enabled once the server fully supports Ephemeral TaskLists as it will otherwise return errors for the unknown TaskListKind. Signed-off-by: Nathanael Mortensen <[email protected]>
Update Sessions to use Ephemeral TaskLists behind a feature flag. This ensures that the per-host TaskList is automatically removed once it is no longer used.
This should only be enabled once the server fully supports Ephemeral TaskLists as it will otherwise return errors for the unknown TaskListKind.
What changed?
Why?
How did you test it?
Potential risks
Detailed Description
See above.
Impact Analysis
Testing Plan
Rollout Plan